Skip to content
This repository has been archived by the owner on Nov 17, 2023. It is now read-only.

Add Median/p50, p90, p99 to python profiler #15953

Merged
merged 6 commits into from
Aug 26, 2019

Conversation

ChaiBapchya
Copy link
Contributor

@ChaiBapchya ChaiBapchya commented Aug 20, 2019

Description

Profile more metrics (than just average; also fix incorrect avg calculation)

Checklist

Essentials

Please feel free to remove inapplicable items for your PR.

  • Changes are complete (i.e. I finished coding on this PR)
  • All changes have test coverage:
  • Code is well-documented:
  • To the my best knowledge, examples are either not affected by this change, or have been fixed to be compatible with this change

end_time = time.perf_counter() # 2
run_time = end_time - start_time # 3
runs = args[1]
modified_args = (args[0], 1)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what is this for?

Copy link
Contributor Author

@ChaiBapchya ChaiBapchya Aug 21, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so Im trying to reuse the existing code.
Other function uses runs and runs a for loop here

https://github.com/ChaiBapchya/incubator-mxnet/blob/bb3e9fc65b756716b4f010c98ed2cfe733f52386/benchmark/opperf/utils/ndarray_utils.py#L45

I reuse the above function. MXNet profiler is able to log the data on a per run basis without explicitly storing that info after every run/iteration

But we need to do that. So that's what I am doing it here

https://github.com/ChaiBapchya/incubator-mxnet/blob/bb3e9fc65b756716b4f010c98ed2cfe733f52386/benchmark/opperf/utils/profiler_utils.py#L236

Not sure if this is the best way. But I wanted to reuse the code and ensure it is backward compatible. Hence this workaround.

Let me know if I should clarify further.

@apeforest
Copy link
Contributor

Thanks for the contribution. How to enable the percentile output for users? Or it comes as default?

@ChaiBapchya
Copy link
Contributor Author

Comes as default if chosen profiler as python

unused var remove

add parameter

missing arg

add mean, median, p90,p99 to markdown

markdown
Copy link
Contributor

@apeforest apeforest left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

end_time = time.perf_counter() # 2
run_time = end_time - start_time # 3
runs = args[1]
modified_args = (args[0], 1, args[2])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you give an example case what are now sent in modified_args ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently when you run opperf
args are - [op, runs, args]
runs is the # of times we want to run this operator

For python profiler to work, we need to call the operator explicitly those many times
Hence runs = args[1] stores the number of times user wanted to run the operator

Now, the alias takes the argument to run MXNet profiler on it
We're overwriting on that, hence we pass run=1 to MXNet profiler
modified args = [op, 1, args]
Does that answer your question?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is where we decide which profiler to call (selects which decorator to use 'cpp' or 'python'

https://github.com/ChaiBapchya/incubator-mxnet/blob/5a56aa3580e2f78a1a78581fe8b8f179962a3a5a/benchmark/opperf/utils/benchmark_utils.py#L56

The reason why I had to modify the args, was because I wanted to reuse the function

https://github.com/ChaiBapchya/incubator-mxnet/blob/cba7c4e360d843d523b29be7fb52fa220fcafa92/benchmark/opperf/utils/ndarray_utils.py#L23

Here it runs the operator runs number of times. But I can't add python time functions here as it will change the behavior of the function for python as well as cpp decorator. Hence I had to override the function by modifying the arguments passed.

@anirudhacharya
Copy link
Member

@mxnet-label-bot add [pr-awaiting-review]

@marcoabreu marcoabreu added the pr-awaiting-review PR is waiting for code review label Aug 26, 2019
@apeforest apeforest merged commit ab60214 into apache:master Aug 26, 2019
@ChaiBapchya ChaiBapchya deleted the median,p50,p99 branch June 24, 2020 00:58
@ChaiBapchya ChaiBapchya changed the title Add Median,p50,p99 to python profiler Add Median/p50, p90, p99 to python profiler Jun 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
pr-awaiting-review PR is waiting for code review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants